fix (GT-96): parameters tab search past first #15
Conversation
|
|
||
| self.wildcard = QtCore.QRegExp() | ||
| def __attrs_post_init__(self): | ||
| super().__init__(self.parent) | ||
| self.wildcard.setPatternSyntax(QtCore.QRegExp.Wildcard) |
There was a problem hiding this comment.
(tl;dr probably do nothing, but I felt like thinking about this so I wrote this up anyways)
So, I just learned that you can pass properties to the constructors with PyQt (python-qt-tools/PyQt5-stubs#147 (comment)). I don't know that I want to use that, or if it works with PySide (maybe relevant someday here, definitely relevant elsewhere for us), but figured I'd share anyways. Point is that this could get moved up to the attribute definition then.
wildcard: QtCore.QRegexp = attr.Factory(lambda: QtCore.QRegexp(patternSyntax=QtCore.QRegexp.Wildcard)But that still doesn't get rid of the .__attrs_post_init__() so... whatever... until we write a qt-specific attrs decorator... someday. Or maybe your change below leads somewhere good. Taking it a step further, perhaps make a basic base class that handles the .__attrs_post_init__() then leave all the other classes clean (at least those that only need parent passed up).
@attr.s(auto_attrib=True)
class AttrsQObject:
_parent: QtWidgets.QWidget = None
def __attrs_post_init__(self):
super().__init__(parent=self._parent)Note the 'private' ._parent to avoid shadowing the QObject.parent() method. Yeah, that issue pre-exists this change, just noticed it. Ah, inheritance...
There was a problem hiding this comment.
I held off changing the wildcard declaration + initialization to a one liner. I did do the _parent. Again that's a good find.
What I found is that no matter what, there is no way to avoid adding the __attrs_post_init__ method. The inherited class init method has to be called and as far as I can tell the only way to do that is making a call through __attrs_post_init__.
Really, it doesn't make much sense to have the base class. It's not saving any lines of code, that's for sure. Let me know what you think.
There was a problem hiding this comment.
Having a general AttrsQObject, or whatever, is a bigger picture thing than this PR or these two classes. The sort of thing that is of at least uncertain value to backport, but ... might (?) be a good thing for future development. It would clearly avoid the .parent shadow issue in one place and allow perhaps many other classes to not have a .__attrs_post_init__() which would encourage them to do less in there which is one of the things I've accepted as good about attrs. No more, 'well I already have one so... do a bit more!'. :]
But yeah, big picture, various opinions, not really to be dealt with in this PR.
|
Also, to answer your question on the need for inheritance. The real fix for this was to ditch inheriting from Now, in my research for this, I did discover this paradigm of foregoing inheritance in Python (and elsewhere) for interfaces. This goes against everything I've been taught. I'm going to explore this paradigm more. |
There was a problem hiding this comment.
I see composition over inheritance as a bit separate from layering the proxies or not. Qt is _all_ about inheritance yet they refer to setting up proxies in pipelines.
https://doc.qt.io/qt-5/model-view-programming.html#using-proxy-models
They can also be used to process the information obtained from other proxy models in a pipeline arrangement.
But sure, that is... not really a 'thou shalt stack proxies and not multiply inherit them'. Just a mild suggestion of one option. :]
Anyways, yeah, a few years ago I accepted the habit of at least tending away from inheritance. So far I haven't regretted it. There are other issues you hit that way, no doubt. And I don't straight up 'ban' inheritance (in so much as I have any such authority anyways). One factor I see is that historically C++ was kind of the big name in OOP and to have a collection of This and That you would have to have had the collection be set to contain some common inheritance ancestor of the two. With Python, it is sufficient that all the objects in the list simply have the expected methods and attributes, if any, that code operating on the collections exercise. tl;dr, inheritance used to be roughly mandatory, now it is more optional. Let's try not-it out.
In this particular case, there really isn't much code to share, I don't think? When the inheritance was switched from QIdentityProxyModel to SortFilterProxyModel only the .__attrs_post_init__() was removed and that is mostly independent of either of these.
Down to the core issue that inspired this PR... past the vast void between the inheritors and the composers... back to an actual problem... :] Do you know what bit of QIdentityProxyModel was causing trouble while our two proxies were still separate?
I just tried here in Linux and noticed the output below. Had you seen this? Maybe it is more about some code I have that is mishandling the two layers of proxies? Switching to inheritance had the effect of reducing down to a single proxy layer. And, I do have messy code that isn't really respecting the intended use of the proxies. :[ Sorry... again. I guess one test would be to just swap inheritance from QIdentityProxyModel to QSortFilterProxyModel and see if it fails the same way as before.
qt_message_handler: f:None l:0 f():None
WARNING: QSortFilterProxyModel: index from wrong model passed to mapToSource
Now that I've suggested more stuff to look into... I'll volunteer to do some of the digging and checking since I'm the one resisting the change here. It just doesn't feel like there _should_ be an issue with QIdentityProxyModel. If nothing else, my compulsion is to know a bit more detail as to why.
If not that, then I dunno, maybe this is somehow related? QIdentityProxyModel::match inappropriately mixes use of proxy model index and source model data method
Side note, maybe we should update PyQt (separately). Looks to be 5.13.0 in stlib and 5.14.1 in st.
|
|
||
| self.wildcard = QtCore.QRegExp() | ||
| def __attrs_post_init__(self): | ||
| super().__init__(self.parent) | ||
| self.wildcard.setPatternSyntax(QtCore.QRegExp.Wildcard) |
There was a problem hiding this comment.
Having a general AttrsQObject, or whatever, is a bigger picture thing than this PR or these two classes. The sort of thing that is of at least uncertain value to backport, but ... might (?) be a good thing for future development. It would clearly avoid the .parent shadow issue in one place and allow perhaps many other classes to not have a .__attrs_post_init__() which would encourage them to do less in there which is one of the things I've accepted as good about attrs. No more, 'well I already have one so... do a bit more!'. :]
But yeah, big picture, various opinions, not really to be dealt with in this PR.
Co-authored-by: Kyle Altendorf <[email protected]>
|
I did notice the warning message that was printing and I no longer get that message during search with this code. The reason I ditched I also made the assumption that the Last point, I'm not sure what |
|
|
|
Per suggestions, I added printing of the stack trace in the message handler and also adjusted the code in the |
There was a problem hiding this comment.
The changes to search_view() were sufficient to fix the issue since they corrected the layered proxy/model traversal. Do we need to couple the two proxies together by inheritance still? A call is welcome if that would make a better discussion. Otherwise I think I'm good.
epyqlib/utils/qt.py
Outdated
| def __init__(self, *args, filter_column, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| @attr.s(auto_attribs=True) | ||
| class BaseSortFilterProxyModel(QtCore.QSortFilterProxyModel): |
There was a problem hiding this comment.
I had imagined this being a generic QObject inheriting thing but I kind of forgot about how we want to inherit from something else... I guess maybe it would still work and then the other class would just get mixed in?
@attr.s(auto_attribs=True)
class AttrsQObject(QtCore.QObject):
_parent: QtCore.QObject = None
def __attrs_post_init__(self):
super().__init__(self._parent)
@attr.s(auto_attribs=True)
class SortFilterProxyModel(AttrsQObject, QtCore.QSortFilterProxyModel):This is really a big picture question though and I can't say I'm hot on the multiple inheritance I just typed here... maybe it isn't worth it and a pattern is the right approach.
On the Parameters tab, allow search to go past the first found item.